Run mypy on CI, fix or ignore typing issues #987

krassowski · 2024-09-10T13:48:48Z

Fixes #985

For now this only runs mypy on jupyter-ai but not yet on jupyter-ai-magics (which we may decide to add in a separate PR).

dlqqq · 2024-09-11T18:31:55Z

@krassowski Thank you for this significant contribution! This was on our to-do list for a while but we never managed to get around to it.

Since this is a large PR that changes many type hints that pydantic relies on, I do want to allow myself more time to test this thoroughly. I plan to review this tomorrow after I cut a release today.

krassowski · 2024-09-11T18:38:16Z

@dlqqq If you mean the ellipses, I think that is a straightforward change. Quoting from pydantic documentation (emphasis mine):

To declare a field as required, you may declare it using an annotation, or an annotation in combination with a Field specification. You may also use Ellipsis/... to emphasize that a field is required, especially when using the Field constructor.

[...]
from pydantic import BaseModel, Field


class Model(BaseModel):
    a: int
    b: int = ...
    c: int = Field(..., alias='C')
Here a, b and c are all required. However, this use of b: int = ... does not work properly with mypy, and as of v1.0 should be avoided in most cases.

https://docs.pydantic.dev/latest/concepts/models/#field-ordering

krassowski · 2024-09-11T18:39:54Z

and as of v1.0 should be avoided in most cases

to be clear, this is verbatim from the latest v2 documentation so should be taken as applying to both v1 and v2 :)

dlqqq · 2024-09-12T16:46:39Z

There's an unrelated error in CI for the E2E tests workflow:

Error: This request has been automatically failed because it uses a deprecated version of `actions/upload-artifact: v2`. Learn more: https://github.blog/changelog/2024-02-13-deprecation-notice-v1-and-v2-of-the-artifact-actions/

I'm looking into this now and will open a separate PR to address this.

dlqqq

This PR looks good to me! However, when I run mypy locally, I get an error regarding the deepmerge package being untyped, even though the same check passes in CI. I'm using mypy 1.11.2, same as the CI env. Not sure what could be causing this 🤔

% mypy packages/jupyter-ai
packages/jupyter-ai/jupyter_ai/config_manager.py:8: error: Skipping analyzing "deepmerge": module is installed, but missing library stubs or py.typed marker  [import-untyped]
packages/jupyter-ai/jupyter_ai/config_manager.py:8: note: See https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-imports
packages/jupyter-ai/jupyter_ai/handlers.py:556: note: By default the bodies of untyped functions are not checked, consider using --check-untyped-defs  [annotation-unchecked]
packages/jupyter-ai/jupyter_ai/extension.py:393: note: By default the bodies of untyped functions are not checked, consider using --check-untyped-defs  [annotation-unchecked]
packages/jupyter-ai/jupyter_ai/extension.py:420: note: By default the bodies of untyped functions are not checked, consider using --check-untyped-defs  [annotation-unchecked]
Found 1 error in 1 file (checked 27 source files)

% mypy --version
mypy 1.11.2 (compiled: yes)

krassowski · 2024-09-12T16:51:55Z

You need to update deepmerge to newer version - this is why there is no error on CI.

krassowski · 2024-09-12T16:52:32Z

See toumorokoshi/deepmerge#30 - this was merged 8 months ago. I also had an old version in my dev env :)

dlqqq · 2024-09-12T18:14:46Z

@krassowski Awesome, thanks for finding that reference so quickly!

One remaining question: Should we bump the version specifier on deepmerge and require deepmerge>=2.0.0? I went through the changelog and it looks safe to me, but I'd like your take on this as well.

krassowski · 2024-09-12T18:24:01Z

My thinking was that an update was not needed because end users do not care about mypy checks on CI, and developers will not get it auto-updated when they pull changes either (unless they reinstall the environment). I wonder if it should have an upper bound on <3 just to ensure a future release does not break installation.

But if you think it is worth pinning the floor, then let's do that!

dlqqq

Thank you for your contribution & your suggestions! I've bumped the version floor and added a version ceiling in the latest commit. Everything looks good. Will merge pending green CI.

* Run mypy on CI * Rename, add mypy to test deps * Fix typing jupyter-ai codebase (mostly) * Three more cases * update deepmerge version specifier --------- Co-authored-by: David L. Qiu <[email protected]>

@learned

* context provider * split base and base command context providers + replacing prompt * comment * only replace prompt if context variable in template * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Run mypy on CI, fix or ignore typing issues (#987) * Run mypy on CI * Rename, add mypy to test deps * Fix typing jupyter-ai codebase (mostly) * Three more cases * update deepmerge version specifier --------- Co-authored-by: David L. Qiu <[email protected]> * context provider * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * mypy * black * modify backtick logic * allow for spaces in filepath * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * refactor * fixes * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * fix test * refactor autocomplete to remove hardcoded '/' and '@' prefix * modify context prompt template Co-authored-by: david qiu <[email protected]> * refactor * docstrings + refactor * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * mypy * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * add context providers to help * remove _examples.py and remove @learned from defaults * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * make find_commands unoverridable --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Michał Krassowski <[email protected]> Co-authored-by: David L. Qiu <[email protected]>

* Run mypy on CI * Rename, add mypy to test deps * Fix typing jupyter-ai codebase (mostly) * Three more cases * update deepmerge version specifier --------- Co-authored-by: David L. Qiu <[email protected]>

@learned

* context provider * split base and base command context providers + replacing prompt * comment * only replace prompt if context variable in template * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Run mypy on CI, fix or ignore typing issues (jupyterlab#987) * Run mypy on CI * Rename, add mypy to test deps * Fix typing jupyter-ai codebase (mostly) * Three more cases * update deepmerge version specifier --------- Co-authored-by: David L. Qiu <[email protected]> * context provider * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * mypy * black * modify backtick logic * allow for spaces in filepath * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * refactor * fixes * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * fix test * refactor autocomplete to remove hardcoded '/' and '@' prefix * modify context prompt template Co-authored-by: david qiu <[email protected]> * refactor * docstrings + refactor * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * mypy * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * add context providers to help * remove _examples.py and remove @learned from defaults * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * make find_commands unoverridable --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Michał Krassowski <[email protected]> Co-authored-by: David L. Qiu <[email protected]>

krassowski added the bug Something isn't working label Sep 10, 2024

krassowski force-pushed the mypy branch 2 times, most recently from f64bd7e to a2e9890 Compare September 10, 2024 14:48

krassowski marked this pull request as ready for review September 10, 2024 14:56

krassowski force-pushed the mypy branch from f61b381 to 2f09333 Compare September 11, 2024 10:29

dlqqq force-pushed the mypy branch from 2f09333 to 55cb53c Compare September 12, 2024 16:39

dlqqq requested changes Sep 12, 2024

View reviewed changes

dlqqq mentioned this pull request Sep 12, 2024

Upgrade to actions/upload-artifact@v4 in workflows #992

Merged

krassowski added 4 commits September 12, 2024 10:29

Run mypy on CI

c0ef239

Rename, add mypy to test deps

3693471

Fix typing jupyter-ai codebase (mostly)

4724161

Three more cases

4f46412

dlqqq force-pushed the mypy branch from 55cb53c to 4f46412 Compare September 12, 2024 17:29

update deepmerge version specifier

19d92e3

dlqqq approved these changes Sep 12, 2024

View reviewed changes

dlqqq enabled auto-merge (squash) September 12, 2024 19:17

dlqqq merged commit 7b4a708 into jupyterlab:main Sep 12, 2024
9 checks passed

dlqqq mentioned this pull request Sep 12, 2024

Adds unix shell-style wildcard matching to /learn #989

Merged

krassowski deleted the mypy branch September 20, 2024 11:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Run mypy on CI, fix or ignore typing issues #987

Run mypy on CI, fix or ignore typing issues #987

krassowski commented Sep 10, 2024 •

edited

Loading

dlqqq commented Sep 11, 2024

krassowski commented Sep 11, 2024 •

edited

Loading

krassowski commented Sep 11, 2024

dlqqq commented Sep 12, 2024 •

edited

Loading

dlqqq left a comment

krassowski commented Sep 12, 2024

krassowski commented Sep 12, 2024

dlqqq commented Sep 12, 2024

krassowski commented Sep 12, 2024

dlqqq left a comment

Run mypy on CI, fix or ignore typing issues #987

Run mypy on CI, fix or ignore typing issues #987

Conversation

krassowski commented Sep 10, 2024 • edited Loading

dlqqq commented Sep 11, 2024

krassowski commented Sep 11, 2024 • edited Loading

krassowski commented Sep 11, 2024

dlqqq commented Sep 12, 2024 • edited Loading

dlqqq left a comment

Choose a reason for hiding this comment

krassowski commented Sep 12, 2024

krassowski commented Sep 12, 2024

dlqqq commented Sep 12, 2024

krassowski commented Sep 12, 2024

dlqqq left a comment

Choose a reason for hiding this comment

krassowski commented Sep 10, 2024 •

edited

Loading

krassowski commented Sep 11, 2024 •

edited

Loading

dlqqq commented Sep 12, 2024 •

edited

Loading